From 40d1c10a839bc628986baf4ac6d7dcd7c56a59f5 Mon Sep 17 00:00:00 2001
From: Alex Crichton <alex@alexcrichton.com>
Date: Mon, 4 May 2015 22:55:03 -0700
Subject: [PATCH] Check default features when skipping resolution of a package

Previously if a package had been activated without the default feature and then
it was attempted to be reactivated with the default feature, resolution wouldn't
continue and actually activate the default feature. This means that the
fingerprint on the other end of compilation would be slightly different because
the set of activated features would be slightly different, causing spurious
recompiles.

Closes #1567
---
 src/cargo/core/resolver/mod.rs           | 32 ++++++++++++-------
 src/cargo/ops/cargo_rustc/fingerprint.rs |  1 +
 tests/test_cargo_features.rs             | 40 ++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 12 deletions(-)

diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs
index d3b58c8fb..2f9bd5725 100644
--- a/src/cargo/core/resolver/mod.rs
+++ b/src/cargo/core/resolver/mod.rs
@@ -112,7 +112,12 @@ impl Resolve {
 
 impl fmt::Debug for Resolve {
     fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
-        self.graph.fmt(fmt)
+        try!(write!(fmt, "graph: {:?}\n", self.graph));
+        try!(write!(fmt, "\nfeatures: {{\n"));
+        for (pkg, features) in &self.features {
+            try!(write!(fmt, "  {}: {:?}\n", pkg, features));
+        }
+        write!(fmt, "}}")
     }
 }
 
@@ -223,12 +228,17 @@ fn flag_activated(cx: &mut Context,
         return false
     }
     debug!("checking if {} is already activated", summary.package_id());
-    let features = match *method {
-        Method::Required{features, ..} => features,
+    let (features, use_default) = match *method {
+        Method::Required { features, uses_default_features, .. } => {
+            (features, uses_default_features)
+        }
         Method::Everything => return false,
     };
     match cx.resolve.features(id) {
-        Some(prev) => features.iter().all(|f| prev.contains(f)),
+        Some(prev) => {
+            features.iter().all(|f| prev.contains(f)) &&
+                (!use_default || prev.contains("default"))
+        }
         None => features.len() == 0,
     }
 }
@@ -422,7 +432,7 @@ fn resolve_features<'a>(cx: &mut Context, parent: &'a Summary,
                                                (&'a Dependency, Vec<String>)>> {
     let dev_deps = match method {
         Method::Everything => true,
-        Method::Required{dev_deps, ..} => dev_deps,
+        Method::Required { dev_deps, .. } => dev_deps,
     };
 
     // First, filter by dev-dependencies
@@ -519,14 +529,13 @@ fn build_features(s: &Summary, method: Method)
     }
     match method {
         Method::Everything |
-        Method::Required{uses_default_features: true, ..} => {
-            if s.features().get("default").is_some() &&
-               !visited.contains("default") {
+        Method::Required { uses_default_features: true, .. } => {
+            if s.features().get("default").is_some() {
                 try!(add_feature(s, "default", &mut deps, &mut used,
                                  &mut visited));
             }
         }
-        _ => {}
+        Method::Required { uses_default_features: false, .. } => {}
     }
     return Ok((deps, used));
 
@@ -560,9 +569,8 @@ fn build_features(s: &Summary, method: Method)
                 used.insert(feat.to_string());
                 match s.features().get(feat) {
                     Some(recursive) => {
-                        for f in recursive.iter() {
-                            try!(add_feature(s, f, deps, used,
-                                             visited));
+                        for f in recursive {
+                            try!(add_feature(s, f, deps, used, visited));
                         }
                     }
                     None => {
diff --git a/src/cargo/ops/cargo_rustc/fingerprint.rs b/src/cargo/ops/cargo_rustc/fingerprint.rs
index 8537974f2..707f0ccb9 100644
--- a/src/cargo/ops/cargo_rustc/fingerprint.rs
+++ b/src/cargo/ops/cargo_rustc/fingerprint.rs
@@ -153,6 +153,7 @@ fn calculate<'a, 'b>(cx: &mut Context<'a, 'b>,
     });
     let extra = util::short_hash(&(cx.config.rustc_version(), target, &features,
                                    profile));
+    debug!("extra {:?} {:?} {:?} = {}", target, profile, features, extra);
 
     // Next, recursively calculate the fingerprint for all of our dependencies.
     //
diff --git a/tests/test_cargo_features.rs b/tests/test_cargo_features.rs
index 10bb898e2..d46d0d7ae 100644
--- a/tests/test_cargo_features.rs
+++ b/tests/test_cargo_features.rs
@@ -655,3 +655,43 @@ test!(everything_in_the_lockfile {
     assert!(lockfile.contains(r#"name = "d2""#), "d2 not found\n{}", lockfile);
     assert!(lockfile.contains(r#"name = "d3""#), "d3 not found\n{}", lockfile);
 });
+
+test!(no_rebuild_when_frobbing_default_feature {
+    let p = project("foo")
+        .file("Cargo.toml", r#"
+            [package]
+            name = "foo"
+            version = "0.1.0"
+            authors = []
+
+            [dependencies]
+            a = { path = "a" }
+            b = { path = "b" }
+        "#)
+        .file("src/lib.rs", "")
+        .file("b/Cargo.toml", r#"
+            [package]
+            name = "b"
+            version = "0.1.0"
+            authors = []
+
+            [dependencies]
+            a = { path = "../a", features = ["f1"], default-features = false }
+        "#)
+        .file("b/src/lib.rs", "")
+        .file("a/Cargo.toml", r#"
+            [package]
+            name = "a"
+            version = "0.1.0"
+            authors = []
+
+            [features]
+            default = ["f1"]
+            f1 = []
+        "#)
+        .file("a/src/lib.rs", "");
+
+    assert_that(p.cargo_process("build"), execs().with_status(0));
+    assert_that(p.cargo("build"), execs().with_status(0).with_stdout(""));
+    assert_that(p.cargo("build"), execs().with_status(0).with_stdout(""));
+});
-- 
2.30.2